zero copy nitros ros - #3537
Open
ashrafk93 wants to merge 2 commits into
Open
Conversation
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
Comment on lines
+1262
to
+1272
| bool BaseRealSenseNode::getNitrosImageFormat( | ||
| const rs2_format& format, std::string& nitros_format, std::string& encoding, unsigned int& bpp) | ||
| { | ||
| // Only the common color formats for now (color-only first cut). NITROS supported-type | ||
| // name (used by ManagedNitrosPublisher / negotiation) + matching sensor_msgs encoding + bpp. | ||
| switch (format) | ||
| { | ||
| case RS2_FORMAT_RGB8: nitros_format = "nitros_image_rgb8"; encoding = sensor_msgs::image_encodings::RGB8; bpp = 3; return true; | ||
| case RS2_FORMAT_BGR8: nitros_format = "nitros_image_bgr8"; encoding = sensor_msgs::image_encodings::BGR8; bpp = 3; return true; | ||
| case RS2_FORMAT_RGBA8: nitros_format = "nitros_image_rgba8"; encoding = sensor_msgs::image_encodings::RGBA8; bpp = 4; return true; | ||
| case RS2_FORMAT_BGRA8: nitros_format = "nitros_image_bgra8"; encoding = sensor_msgs::image_encodings::BGRA8; bpp = 4; return true; |
There was a problem hiding this comment.
getNitrosImageFormat duplicates rs2->ROS encoding mappings already defined in initializeFormatsMaps; consolidate the mapping to a single source to avoid divergent updates.
Show fix
Suggested change
| bool BaseRealSenseNode::getNitrosImageFormat( | |
| const rs2_format& format, std::string& nitros_format, std::string& encoding, unsigned int& bpp) | |
| { | |
| // Only the common color formats for now (color-only first cut). NITROS supported-type | |
| // name (used by ManagedNitrosPublisher / negotiation) + matching sensor_msgs encoding + bpp. | |
| switch (format) | |
| { | |
| case RS2_FORMAT_RGB8: nitros_format = "nitros_image_rgb8"; encoding = sensor_msgs::image_encodings::RGB8; bpp = 3; return true; | |
| case RS2_FORMAT_BGR8: nitros_format = "nitros_image_bgr8"; encoding = sensor_msgs::image_encodings::BGR8; bpp = 3; return true; | |
| case RS2_FORMAT_RGBA8: nitros_format = "nitros_image_rgba8"; encoding = sensor_msgs::image_encodings::RGBA8; bpp = 4; return true; | |
| case RS2_FORMAT_BGRA8: nitros_format = "nitros_image_bgra8"; encoding = sensor_msgs::image_encodings::BGRA8; bpp = 4; return true; | |
| bool BaseRealSenseNode::getNitrosImageFormat( | |
| // Use the existing rs2_format to ROS encoding mapping from initializeFormatsMaps. | |
| auto it = _rs_format_to_ros_format.find(format); | |
| if (it == _rs_format_to_ros_format.end()) | |
| return false; | |
| encoding = it->second; | |
| switch (format) | |
| { | |
| case RS2_FORMAT_RGB8: nitros_format = "nitros_image_rgb8"; bpp = 3; return true; | |
| case RS2_FORMAT_BGR8: nitros_format = "nitros_image_bgr8"; bpp = 3; return true; | |
| case RS2_FORMAT_RGBA8: nitros_format = "nitros_image_rgba8"; bpp = 4; return true; | |
| case RS2_FORMAT_BGRA8: nitros_format = "nitros_image_bgra8"; bpp = 4; return true; |
Details
✨ AI Reasoning
The change added a new function that maps several rs2 pixel formats to sensor_msgs encodings and bytes-per-pixel. The same rs2->encoding mappings already exist in initializeFormatsMaps earlier in the file. This is an introduced, localized duplication of mapping logic: updates to supported formats would need changes in two places. Consolidation into a single authoritative mapping would avoid divergence and reduce maintenance burden.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
There was a problem hiding this comment.
Pull request overview
Adds an opt-in NITROS (Isaac ROS) publishing path for the color stream so downstream graphs can consume GPU-resident images without the sensor_msgs/Image → DDS → cudaMemcpy round-trip, while keeping the default ROS image topics unchanged when the feature is disabled.
Changes:
- Introduces a
NitrosImagePublisherwrapper aroundManagedNitrosPublisher<NitrosImage>and publishes on~/color/nitros_imagewhen enabled. - Hooks NITROS publishing into
BaseRealSenseNode::publishFrame()behindBUILD_WITH_NITROSand a newenable_color_nitrosparameter. - Adds a
BUILD_WITH_NITROSCMake option with CUDA + Isaac ROS package discovery/linking.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| realsense2_camera/src/rs_node_setup.cpp | Creates per-stream NITROS publisher for the color stream when enabled. |
| realsense2_camera/src/parameters.cpp | Adds enable_color_nitros parameter (compiled only with BUILD_WITH_NITROS). |
| realsense2_camera/src/nitros_image_publisher.cpp | Implements GPU-buffer allocation/copy and NITROS NitrosImage publication. |
| realsense2_camera/src/base_realsense_node.cpp | Calls NITROS publish path from publishFrame(); adds format mapping + publish helper. |
| realsense2_camera/package.xml | Documents optional Isaac ROS/NITROS dependency model (comment-only). |
| realsense2_camera/include/nitros_image_publisher.h | Declares the NITROS publisher wrapper. |
| realsense2_camera/include/constants.h | Adds ROS_WARN_STREAM_ONCE macro used by the new NITROS path. |
| realsense2_camera/include/base_realsense_node.h | Declares NITROS helpers and stores NITROS publishers/flag when enabled. |
| realsense2_camera/CMakeLists.txt | Adds BUILD_WITH_NITROS option, sources/includes, find/link steps for CUDA + NITROS. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+19
to
+24
| #include <isaac_ros_managed_nitros/managed_nitros_publisher.hpp> | ||
| #include <isaac_ros_nitros_image_type/nitros_image.hpp> | ||
| #include <isaac_ros_nitros_image_type/nitros_image_builder.hpp> | ||
|
|
||
| #include <cuda_runtime.h> | ||
| #include <utility> |
Comment on lines
+76
to
+88
| try { | ||
| NitrosImage img = NitrosImageBuilder() | ||
| .WithHeader(header) | ||
| .WithEncoding(encoding) | ||
| .WithDimensions(height, width) | ||
| .WithGpuData(dev) // ownership transfers to GXF (frees via cudaFree on release) | ||
| .Build(); | ||
| _impl->pub->publish(std::move(img)); | ||
| } catch (const std::exception & e) { | ||
| // Build() throws (e.g. odd dimensions / unsupported encoding) before taking ownership. | ||
| cudaFree(dev); | ||
| RCLCPP_WARN(rclcpp::get_logger("NitrosImagePublisher"), "NitrosImage Build failed: %s", e.what()); | ||
| } |
Addresses Copilot review: we catch std::exception, so include the header directly instead of relying on a transitive include. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ashrafk93
force-pushed
the
ashraf/zerocopy-nitors
branch
from
August 18, 2026 06:27
4fb1114 to
fbd0068
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add optional NITROS (GPU zero-copy) color publishing
Opt-in path to publish the color stream to an Isaac ROS / NITROS graph on the GPU, skipping the
sensor_msgs/Image→ DDS →cudaMemcpyround-trip. Additive and gated — default builds and~/color/image_raware unchanged. (RSDEV-6254)What's new
nitros_image_publisher.{h,cpp}— wrapsManagedNitrosPublisher<NitrosImage>; publishes on~/color/nitros_image.publishNitrosFrame()hooked intopublishFrame();enable_color_nitrosparam (default off).BUILD_WITH_NITROSoption (default OFF) → finds/links CUDA +isaac_ros_nitrospackages.Enable
Needs an Isaac ROS workspace + librealsense built with
BUILD_WITH_CUDA_ZEROCOPY.Tested (Orin, Isaac ROS 3.2, D435)
Builds; publishes ~28 Hz with REP-2007/2009 negotiation; zero-copy source confirmed. End-to-end @1080p: NITROS consumer holds 28.5 FPS vs baseline ~27 FPS (drops frames, ~55 ms latency).